-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[HLSL][Sema] Use hlsl::BindingInfoBuilder instead of RangeInfo. NFC #150634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
064bfb3
to
63a91eb
Compare
Clean up some duplicated logic. We had two ways to do the same thing here, and BindingInfoBuilder is more flexible.
63a91eb
to
6b4301e
Compare
@llvm/pr-subscribers-clang @llvm/pr-subscribers-hlsl Author: Justin Bogner (bogner) ChangesClean up some duplicated logic. We had two ways to do the same thing Patch is 28.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/150634.diff 5 Files Affected:
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index 9276554bebf9d..de326f99bbbbd 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -39,6 +39,7 @@
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/Twine.h"
+#include "llvm/Frontend/HLSL/HLSLBinding.h"
#include "llvm/Frontend/HLSL/RootSignatureValidations.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/DXILABI.h"
@@ -1083,6 +1084,102 @@ void SemaHLSL::ActOnFinishRootSignatureDecl(
SemaRef.PushOnScopeChains(SignatureDecl, SemaRef.getCurScope());
}
+namespace {
+
+struct PerVisibilityBindingChecker {
+ SemaHLSL *S;
+ // We need one builder per `llvm::dxbc::ShaderVisibility` value.
+ std::array<llvm::hlsl::BindingInfoBuilder, 8> Builders;
+
+ struct ElemInfo {
+ const hlsl::RootSignatureElement *Elem;
+ llvm::dxbc::ShaderVisibility Vis;
+ bool Diagnosed;
+ };
+ llvm::SmallVector<ElemInfo> ElemInfoMap;
+
+ PerVisibilityBindingChecker(SemaHLSL *S) : S(S) {}
+
+ void trackBinding(llvm::dxbc::ShaderVisibility Visibility,
+ llvm::dxil::ResourceClass RC, uint32_t Space,
+ uint32_t LowerBound, uint32_t UpperBound,
+ const hlsl::RootSignatureElement *Elem) {
+ uint32_t BuilderIndex = llvm::to_underlying(Visibility);
+ assert(BuilderIndex < Builders.size() &&
+ "Not enough builders for visibility type");
+ Builders[BuilderIndex].trackBinding(RC, Space, LowerBound, UpperBound,
+ static_cast<const void *>(Elem));
+
+ static_assert(llvm::to_underlying(llvm::dxbc::ShaderVisibility::All) == 0,
+ "'All' visibility must come first");
+ if (Visibility == llvm::dxbc::ShaderVisibility::All)
+ for (size_t I = 1, E = Builders.size(); I < E; ++I)
+ Builders[I].trackBinding(RC, Space, LowerBound, UpperBound,
+ static_cast<const void *>(Elem));
+
+ ElemInfoMap.push_back({Elem, Visibility, false});
+ }
+
+ ElemInfo &getInfo(const hlsl::RootSignatureElement *Elem) {
+ auto It = llvm::lower_bound(
+ ElemInfoMap, Elem,
+ [](const auto &LHS, const auto &RHS) { return LHS.Elem < RHS; });
+ assert(It->Elem == Elem && "Element not in map");
+ return *It;
+ }
+
+ bool checkOverlap() {
+ llvm::sort(ElemInfoMap, [](const auto &LHS, const auto &RHS) {
+ return LHS.Elem < RHS.Elem;
+ });
+
+ bool HadOverlap = false;
+
+ using llvm::hlsl::BindingInfoBuilder;
+ auto ReportOverlap = [this, &HadOverlap](
+ const BindingInfoBuilder &Builder,
+ const BindingInfoBuilder::Binding &Reported) {
+ HadOverlap = true;
+
+ const auto *Elem =
+ static_cast<const hlsl::RootSignatureElement *>(Reported.Cookie);
+ const BindingInfoBuilder::Binding &Previous =
+ Builder.findOverlapping(Reported);
+ const auto *PrevElem =
+ static_cast<const hlsl::RootSignatureElement *>(Previous.Cookie);
+
+ ElemInfo &Info = getInfo(Elem);
+ // We will have already diagnosed this binding if there's overlap in the
+ // "All" visibility as well as any particular visibility.
+ if (Info.Diagnosed)
+ return;
+ Info.Diagnosed = true;
+
+ ElemInfo &PrevInfo = getInfo(PrevElem);
+ llvm::dxbc::ShaderVisibility CommonVis =
+ Info.Vis == llvm::dxbc::ShaderVisibility::All ? PrevInfo.Vis
+ : Info.Vis;
+
+ this->S->Diag(Elem->getLocation(), diag::err_hlsl_resource_range_overlap)
+ << llvm::to_underlying(Reported.RC) << Reported.LowerBound
+ << Reported.isUnbounded() << Reported.UpperBound
+ << llvm::to_underlying(Previous.RC) << Previous.LowerBound
+ << Previous.isUnbounded() << Previous.UpperBound << Reported.Space
+ << CommonVis;
+
+ this->S->Diag(PrevElem->getLocation(),
+ diag::note_hlsl_resource_range_here);
+ };
+
+ for (BindingInfoBuilder &Builder : Builders)
+ Builder.calculateBindingInfo(ReportOverlap);
+
+ return HadOverlap;
+ }
+};
+
+} // end anonymous namespace
+
bool SemaHLSL::handleRootSignatureElements(
ArrayRef<hlsl::RootSignatureElement> Elements) {
// Define some common error handling functions
@@ -1171,147 +1268,67 @@ bool SemaHLSL::handleRootSignatureElements(
}
}
- using RangeInfo = llvm::hlsl::rootsig::RangeInfo;
- using OverlappingRanges = llvm::hlsl::rootsig::OverlappingRanges;
- using InfoPairT = std::pair<RangeInfo, const hlsl::RootSignatureElement *>;
+ PerVisibilityBindingChecker BindingChecker(this);
+ SmallVector<std::pair<const llvm::hlsl::rootsig::DescriptorTableClause *,
+ const hlsl::RootSignatureElement *>>
+ UnboundClauses;
- // 1. Collect RangeInfos
- llvm::SmallVector<InfoPairT> InfoPairs;
for (const hlsl::RootSignatureElement &RootSigElem : Elements) {
const llvm::hlsl::rootsig::RootElement &Elem = RootSigElem.getElement();
if (const auto *Descriptor =
std::get_if<llvm::hlsl::rootsig::RootDescriptor>(&Elem)) {
- RangeInfo Info;
- Info.LowerBound = Descriptor->Reg.Number;
- Info.UpperBound = Info.LowerBound; // use inclusive ranges []
-
- Info.Class =
- llvm::dxil::ResourceClass(llvm::to_underlying(Descriptor->Type));
- Info.Space = Descriptor->Space;
- Info.Visibility = Descriptor->Visibility;
+ uint32_t LowerBound(Descriptor->Reg.Number);
+ uint32_t UpperBound(LowerBound); // inclusive range
- InfoPairs.push_back({Info, &RootSigElem});
+ BindingChecker.trackBinding(
+ Descriptor->Visibility,
+ static_cast<llvm::dxil::ResourceClass>(Descriptor->Type),
+ Descriptor->Space, LowerBound, UpperBound, &RootSigElem);
} else if (const auto *Constants =
std::get_if<llvm::hlsl::rootsig::RootConstants>(&Elem)) {
- RangeInfo Info;
- Info.LowerBound = Constants->Reg.Number;
- Info.UpperBound = Info.LowerBound; // use inclusive ranges []
+ uint32_t LowerBound(Constants->Reg.Number);
+ uint32_t UpperBound(LowerBound); // inclusive range
- Info.Class = llvm::dxil::ResourceClass::CBuffer;
- Info.Space = Constants->Space;
- Info.Visibility = Constants->Visibility;
-
- InfoPairs.push_back({Info, &RootSigElem});
+ BindingChecker.trackBinding(
+ Constants->Visibility, llvm::dxil::ResourceClass::CBuffer,
+ Constants->Space, LowerBound, UpperBound, &RootSigElem);
} else if (const auto *Sampler =
std::get_if<llvm::hlsl::rootsig::StaticSampler>(&Elem)) {
- RangeInfo Info;
- Info.LowerBound = Sampler->Reg.Number;
- Info.UpperBound = Info.LowerBound; // use inclusive ranges []
-
- Info.Class = llvm::dxil::ResourceClass::Sampler;
- Info.Space = Sampler->Space;
- Info.Visibility = Sampler->Visibility;
+ uint32_t LowerBound(Sampler->Reg.Number);
+ uint32_t UpperBound(LowerBound); // inclusive range
- InfoPairs.push_back({Info, &RootSigElem});
+ BindingChecker.trackBinding(
+ Sampler->Visibility, llvm::dxil::ResourceClass::Sampler,
+ Sampler->Space, LowerBound, UpperBound, &RootSigElem);
} else if (const auto *Clause =
std::get_if<llvm::hlsl::rootsig::DescriptorTableClause>(
&Elem)) {
- RangeInfo Info;
- Info.LowerBound = Clause->Reg.Number;
- // Relevant error will have already been reported above and needs to be
- // fixed before we can conduct range analysis, so shortcut error return
- if (Clause->NumDescriptors == 0)
- return true;
- Info.UpperBound = Clause->NumDescriptors == RangeInfo::Unbounded
- ? RangeInfo::Unbounded
- : Info.LowerBound + Clause->NumDescriptors -
- 1; // use inclusive ranges []
-
- Info.Class = Clause->Type;
- Info.Space = Clause->Space;
-
- // Note: Clause does not hold the visibility this will need to
- InfoPairs.push_back({Info, &RootSigElem});
+ // We'll process these once we see the table element.
+ UnboundClauses.emplace_back(Clause, &RootSigElem);
} else if (const auto *Table =
std::get_if<llvm::hlsl::rootsig::DescriptorTable>(&Elem)) {
- // Table holds the Visibility of all owned Clauses in Table, so iterate
- // owned Clauses and update their corresponding RangeInfo
- assert(Table->NumClauses <= InfoPairs.size() && "RootElement");
- // The last Table->NumClauses elements of Infos are the owned Clauses
- // generated RangeInfo
- auto TableInfos =
- MutableArrayRef<InfoPairT>(InfoPairs).take_back(Table->NumClauses);
- for (InfoPairT &Pair : TableInfos)
- Pair.first.Visibility = Table->Visibility;
- }
- }
-
- // 2. Sort with the RangeInfo <operator to prepare it for findOverlapping
- llvm::sort(InfoPairs,
- [](InfoPairT A, InfoPairT B) { return A.first < B.first; });
-
- llvm::SmallVector<RangeInfo> Infos;
- for (const InfoPairT &Pair : InfoPairs)
- Infos.push_back(Pair.first);
-
- // Helpers to report diagnostics
- uint32_t DuplicateCounter = 0;
- using ElemPair = std::pair<const hlsl::RootSignatureElement *,
- const hlsl::RootSignatureElement *>;
- auto GetElemPair = [&Infos, &InfoPairs, &DuplicateCounter](
- OverlappingRanges Overlap) -> ElemPair {
- // Given we sorted the InfoPairs (and by implication) Infos, and,
- // that Overlap.B is the item retrieved from the ResourceRange. Then it is
- // guarenteed that Overlap.B <= Overlap.A.
- //
- // So we will find Overlap.B first and then continue to find Overlap.A
- // after
- auto InfoB = std::lower_bound(Infos.begin(), Infos.end(), *Overlap.B);
- auto DistB = std::distance(Infos.begin(), InfoB);
- auto PairB = InfoPairs.begin();
- std::advance(PairB, DistB);
-
- auto InfoA = std::lower_bound(InfoB, Infos.end(), *Overlap.A);
- // Similarily, from the property that we have sorted the RangeInfos,
- // all duplicates will be processed one after the other. So
- // DuplicateCounter can be re-used for each set of duplicates we
- // encounter as we handle incoming errors
- DuplicateCounter = InfoA == InfoB ? DuplicateCounter + 1 : 0;
- auto DistA = std::distance(InfoB, InfoA) + DuplicateCounter;
- auto PairA = PairB;
- std::advance(PairA, DistA);
-
- return {PairA->second, PairB->second};
- };
-
- auto ReportOverlap = [this, &GetElemPair](OverlappingRanges Overlap) {
- auto Pair = GetElemPair(Overlap);
- const RangeInfo *Info = Overlap.A;
- const hlsl::RootSignatureElement *Elem = Pair.first;
- const RangeInfo *OInfo = Overlap.B;
-
- auto CommonVis = Info->Visibility == llvm::dxbc::ShaderVisibility::All
- ? OInfo->Visibility
- : Info->Visibility;
- this->Diag(Elem->getLocation(), diag::err_hlsl_resource_range_overlap)
- << llvm::to_underlying(Info->Class) << Info->LowerBound
- << /*unbounded=*/(Info->UpperBound == RangeInfo::Unbounded)
- << Info->UpperBound << llvm::to_underlying(OInfo->Class)
- << OInfo->LowerBound
- << /*unbounded=*/(OInfo->UpperBound == RangeInfo::Unbounded)
- << OInfo->UpperBound << Info->Space << CommonVis;
-
- const hlsl::RootSignatureElement *OElem = Pair.second;
- this->Diag(OElem->getLocation(), diag::note_hlsl_resource_range_here);
- };
-
- // 3. Invoke find overlapping ranges
- llvm::SmallVector<OverlappingRanges> Overlaps =
- llvm::hlsl::rootsig::findOverlappingRanges(Infos);
- for (OverlappingRanges Overlap : Overlaps)
- ReportOverlap(Overlap);
+ assert(UnboundClauses.size() == Table->NumClauses &&
+ "Wrong number of clauses in table?");
+ for (const auto &[Clause, ClauseElem] : UnboundClauses) {
+ uint32_t LowerBound(Clause->Reg.Number);
+ // Relevant error will have already been reported above and needs to be
+ // fixed before we can conduct range analysis, so shortcut error return
+ if (Clause->NumDescriptors == 0)
+ return true;
+ uint32_t UpperBound = Clause->NumDescriptors == ~0u
+ ? ~0u
+ : LowerBound + Clause->NumDescriptors - 1;
+
+ BindingChecker.trackBinding(
+ Table->Visibility,
+ static_cast<llvm::dxil::ResourceClass>(Clause->Type), Clause->Space,
+ LowerBound, UpperBound, ClauseElem);
+ }
+ UnboundClauses.clear();
+ }
+ }
- return Overlaps.size() != 0;
+ return BindingChecker.checkOverlap();
}
void SemaHLSL::handleRootSignatureAttr(Decl *D, const ParsedAttr &AL) {
diff --git a/llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h b/llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h
index f1e223da95241..fde32a1fff591 100644
--- a/llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h
+++ b/llvm/include/llvm/Frontend/HLSL/RootSignatureValidations.h
@@ -41,114 +41,6 @@ LLVM_ABI bool verifyComparisonFunc(uint32_t ComparisonFunc);
LLVM_ABI bool verifyBorderColor(uint32_t BorderColor);
LLVM_ABI bool verifyLOD(float LOD);
-struct RangeInfo {
- const static uint32_t Unbounded = ~0u;
-
- // Interval information
- uint32_t LowerBound;
- uint32_t UpperBound;
-
- // Information retained for determining overlap
- llvm::dxil::ResourceClass Class;
- uint32_t Space;
- llvm::dxbc::ShaderVisibility Visibility;
-
- bool operator==(const RangeInfo &RHS) const {
- return std::tie(LowerBound, UpperBound, Class, Space, Visibility) ==
- std::tie(RHS.LowerBound, RHS.UpperBound, RHS.Class, RHS.Space,
- RHS.Visibility);
- }
-
- bool operator<(const RangeInfo &RHS) const {
- return std::tie(Class, Space, LowerBound, UpperBound, Visibility) <
- std::tie(RHS.Class, RHS.Space, RHS.LowerBound, RHS.UpperBound,
- RHS.Visibility);
- }
-};
-
-class ResourceRange {
-public:
- using MapT = llvm::IntervalMap<uint32_t, const RangeInfo *, 16,
- llvm::IntervalMapInfo<uint32_t>>;
-
-private:
- MapT Intervals;
-
-public:
- ResourceRange(MapT::Allocator &Allocator) : Intervals(MapT(Allocator)) {}
-
- // Returns a reference to the first RangeInfo that overlaps with
- // [Info.LowerBound;Info.UpperBound], or, std::nullopt if there is no overlap
- LLVM_ABI std::optional<const RangeInfo *>
- getOverlapping(const RangeInfo &Info) const;
-
- // Return the mapped RangeInfo at X or nullptr if no mapping exists
- LLVM_ABI const RangeInfo *lookup(uint32_t X) const;
-
- // Removes all entries of the ResourceRange
- LLVM_ABI void clear();
-
- // Insert the required (sub-)intervals such that the interval of [a;b] =
- // [Info.LowerBound, Info.UpperBound] is covered and points to a valid
- // RangeInfo &.
- //
- // For instance consider the following chain of inserting RangeInfos with the
- // intervals denoting the Lower/Upper-bounds:
- //
- // A = [0;2]
- // insert(A) -> false
- // intervals: [0;2] -> &A
- // B = [5;7]
- // insert(B) -> false
- // intervals: [0;2] -> &A, [5;7] -> &B
- // C = [4;7]
- // insert(C) -> true
- // intervals: [0;2] -> &A, [4;7] -> &C
- // D = [1;5]
- // insert(D) -> true
- // intervals: [0;2] -> &A, [3;3] -> &D, [4;7] -> &C
- // E = [0;unbounded]
- // insert(E) -> true
- // intervals: [0;unbounded] -> E
- //
- // Returns a reference to the first RangeInfo that overlaps with
- // [Info.LowerBound;Info.UpperBound], or, std::nullopt if there is no overlap
- // (equivalent to getOverlapping)
- LLVM_ABI std::optional<const RangeInfo *> insert(const RangeInfo &Info);
-};
-
-struct OverlappingRanges {
- const RangeInfo *A;
- const RangeInfo *B;
-
- OverlappingRanges(const RangeInfo *A, const RangeInfo *B) : A(A), B(B) {}
-};
-
-/// The following conducts analysis on resource ranges to detect and report
-/// any overlaps in resource ranges.
-///
-/// A resource range overlaps with another resource range if they have:
-/// - equivalent ResourceClass (SRV, UAV, CBuffer, Sampler)
-/// - equivalent resource space
-/// - overlapping visbility
-///
-/// The algorithm is implemented in the following steps:
-///
-/// 1. The user will collect RangeInfo from relevant RootElements:
-/// - RangeInfo will retain the interval, ResourceClass, Space and Visibility
-/// - It will also contain an index so that it can be associated to
-/// additional diagnostic information
-/// 2. The user is required to sort the RangeInfo's such that they are grouped
-/// together by ResourceClass and Space
-/// 3. Iterate through the collected RangeInfos by their groups
-/// - For each group we will have a ResourceRange for each visibility
-/// - As we iterate through we will:
-/// A: Insert the current RangeInfo into the corresponding Visibility
-/// ResourceRange
-/// B: Check for overlap with any overlapping Visibility ResourceRange
-LLVM_ABI llvm::SmallVector<OverlappingRanges>
-findOverlappingRanges(ArrayRef<RangeInfo> Infos);
-
} // namespace rootsig
} // namespace hlsl
} // namespace llvm
diff --git a/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp b/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
index f11c7d2033bfb..9d84aa838f476 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureValidations.cpp
@@ -180,140 +180,6 @@ bool verifyBorderColor(uint32_t BorderColor) {
bool verifyLOD(float LOD) { return !std::isnan(LOD); }
-std::optional<const RangeInfo *>
-ResourceRange::getOverlapping(const RangeInfo &Info) const {
- MapT::const_iterator Interval = Intervals.find(Info.LowerBound);
- if (!Interval.valid() || Info.UpperBound < Interval.start())
- return std::nullopt;
- return Interval.value();
-}
-
-const RangeInfo *ResourceRange::lookup(uint32_t X) const {
- return Intervals.lookup(X, nullptr);
-}
-
-void ResourceRange::clear() { return Intervals.clear(); }
-
-std::optional<const RangeInfo *> ResourceRange::insert(const RangeInfo &Info) {
- uint32_t LowerBound = Info.LowerBound;
- uint32_t UpperBound = Info.UpperBound;
-
- std::optional<const RangeInfo *> Res = std::nullopt;
- MapT::iterator Interval = Intervals.begin();
-
- while (true) {
- if (UpperBound < LowerBound)
- break;
-
- Interval.advanceTo(LowerBound);
- if (!Interval.valid()) // No interval found
- break;
-
- // Let Interval = [x;y] and [LowerBound;UpperBound] = [a;b] and note that
- // a <= y implicitly from Intervals.find(LowerBound)
- if (UpperBound < Interval.start())
- break; // found interval does not overlap with inserted one
-
- if (!Res.has_value()) // Update to be the first found intersection
- Res = Interval.value();
-
- if (Interval.start() <= LowerBound && UpperBound <= Interval.stop()) {
- // x <= a <= b <= y implies that [a;b] is covered by [x;y]
- // -> so we don't need to insert this, report an overlap
- return Res;
- } else if (LowerBound <= Interval.start() &&
- Interval.stop() <= UpperBound) {
- // a <= x <= y <= b implies that [x;y] is covered by [a;b]
- // -> so remove the existing interval that we will cover with the
- // overwrite
- Interval.erase();
- } else if (LowerBound < Interval.start() && UpperBound <= Interval.stop()) {
- // a < x <= b <= y implies that [a; x] is not covered but [x;b] is
- // -> so set b = x - 1 such that [a;x-1] is now the interval to insert
- UpperBound = Interval.start() - 1;
- } else if (Interval.start() <= LowerBound && Interval.stop() < UpperBound) {
- // a < x <= b <= y implies that [y; b] is not covered but [a;y] is
- // -> so set a = y + 1 such that [y+1;b] is now the interval to insert
- LowerBound = Interval.stop() + 1;
- }
- }
-
- assert(LowerBound <= UpperBound && "Attempting to insert an empty interval");
- Intervals.insert(LowerBound, UpperBound, &Info);
- return Res;
-}
-
-llvm::SmallVector<OverlappingRanges>
-findOverlappingRanges(ArrayRef<RangeInfo> Infos) {
- // It is expected t...
[truncated]
|
inbelic
approved these changes
Jul 31, 2025
✅ With the latest revision this PR passed the C/C++ code formatter. |
ef6557f
to
4016876
Compare
farzonl
approved these changes
Aug 5, 2025
krishna2803
pushed a commit
to krishna2803/llvm-project
that referenced
this pull request
Aug 12, 2025
…lvm#150634) Clean up some duplicated logic. We had two ways to do the same thing here, and BindingInfoBuilder is more flexible.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
clang:frontend
Language frontend issues, e.g. anything involving "Sema"
clang
Clang issues not falling into any other category
HLSL
HLSL Language Support
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clean up some duplicated logic. We had two ways to do the same thing
here, and BindingInfoBuilder is more flexible.